CASSGO-26 consistency serial was added #1837

OleksiienkoMykyta · 2024-10-28T15:27:02Z

Closes #1832
With the previous implementation, setting consistency SERIAL for Paxos reads was impossible. Fixed in this PR

joao-r-reis

Can't we just remove SerialConsistency altogether and use Consistency everywhere with these changes? I think it would make no difference and we'd get rid of a type that no longer has a reason to exist. Also we're missing a check on the ClusterConfig serial consistency to ensure it's a valid value.

On the other hand we could also keep SerialConsistency around to ensure that only SERIAL or LOCAL_SERIAL can be used when setting the serial consistency level. But to do this we couldn't make it type SerialConsistency = Consistency like it is on this PR. We'd have to keep the previous code for SerialConsistency AND add a Serial+LocalSerial for the Consistency type...

I believe that other drivers don't have a separate consistency level type for SERIAL CONSISTENCY so I'm +1 on the first option (i.e. remove SerialConsistency completely and just use Consistency everywhere with appropriate checks when setting the serial consistency).

OleksiienkoMykyta · 2024-11-08T13:43:40Z

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency.
I added the check to make sure that consistency is valid.

joao-r-reis · 2024-11-08T15:11:13Z

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency. I added the check to make sure that consistency is valid.

Hmm what about if we have type SerialConsistency = Consistency like you did on this PR AND we replace all references of the SerialConsistency type (on parameters etc.) to Consistency? In this scenario I don't think there will be a breaking change. And we can mark SerialConsistency as deprecated

joao-r-reis · 2024-11-08T15:12:05Z

So basically the only change we would add to this PR is make sure that SerialConsistency is not referenced anywhere (replacing with Consistency) and marking SerialConsistency as deprecated

joao-r-reis · 2024-11-08T16:37:40Z

session.go

 	}

+	if cfg.Consistency.IsSerial() {
+		return nil, fmt.Errorf("the default consistency level is not allowed to be SERIAL or LOCAL_SERIAL. Recived value: %v", cfg.Consistency)


No, it's fine if Consistency has SERIAL or LOCAL_SERIAL (it won't work with writes but sure, maybe the app only has reads). I meant that cfg.SerialConsistency has to be SERIAL or LOCAL_SERIAL

I agree, it slipped my mind that consistency could be SERIAL for read operation, thank you for noticing it.

joao-r-reis · 2024-11-12T15:41:30Z

Looks good, please format your commit message following the rules stated in CONTRIBUTING.md and add this JIRA to the CHANGELOG.md in the Added section of Unreleased, thanks!

We need another committer +1 to merge this.

OleksiienkoMykyta · 2024-11-12T16:02:48Z

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency. I added the check to make sure that consistency is valid.

Hmm what about if we have type SerialConsistency = Consistency like you did on this PR AND we replace all references of the SerialConsistency type (on parameters etc.) to Consistency? In this scenario I don't think there will be a breaking change. And we can mark SerialConsistency as deprecated

I like this idea, it will solve the issue

ribaraka · 2024-11-13T15:26:07Z

frame.go

 	EachQuorum  Consistency = 0x07
 	LocalOne    Consistency = 0x0A
+	Serial      Consistency = 0x08
+	LocalSerial Consistency = 0x09


NIT: move Serial and LocalSerial above LocalOne to maintain sequential order.

ribaraka · 2024-11-13T15:28:34Z

session.go

+	if cfg.SerialConsistency > 0 && !cfg.SerialConsistency.IsSerial() {
+		return nil, fmt.Errorf("the default SerialConsistency level is not allowed to be anything else but SERIAL or LOCAL_SERIAL. Recived value: %v", cfg.SerialConsistency)
+	}


How about adding a test for this?

And overall, how about adding another test to verify that these two consistency levels are allowed for use?

I have added some tests to cover each consistency level, please check it

jameshartig

Overall, I like it, let's just make that method be private if we don't need to expose it.

frame.go

jameshartig

One small thing

frame.go

jameshartig · 2025-03-02T19:40:36Z

@OleksiienkoMykyta There's a merge conflict then this can be merged

The user should be able to set consistency to SERIAL or LOCAL_SERIAL for Paxos reads, but the previous implementation doesn't support such a feature. patch by Mykyta Oleksiienko; reviewed by João Reis and James Harting for CASSGO-26

OleksiienkoMykyta · 2025-03-03T07:39:13Z

@OleksiienkoMykyta There's a merge conflict then this can be merged

fixed

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from 76a1c5f to 2d83ec8 Compare October 28, 2024 15:29

joao-r-reis reviewed Nov 5, 2024

View reviewed changes

joao-r-reis changed the title ~~consistency serial was added~~ CASSGO-26 consistency serial was added Nov 5, 2024

OleksiienkoMykyta requested a review from joao-r-reis November 8, 2024 13:48

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from 2d83ec8 to 3662be4 Compare November 8, 2024 13:51

joao-r-reis reviewed Nov 8, 2024

View reviewed changes

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from 3662be4 to be2bc58 Compare November 12, 2024 15:24

joao-r-reis approved these changes Nov 12, 2024

View reviewed changes

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch 3 times, most recently from accb87e to a18e16e Compare November 13, 2024 09:16

ribaraka reviewed Nov 13, 2024

View reviewed changes

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch 2 times, most recently from 57f784b to 769a5c1 Compare November 14, 2024 11:05

OleksiienkoMykyta requested a review from ribaraka November 14, 2024 11:07

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from 769a5c1 to e963b69 Compare November 14, 2024 12:54

ribaraka approved these changes Nov 14, 2024

View reviewed changes

OleksiienkoMykyta mentioned this pull request Nov 21, 2024

CASSGO-29 refactor packages to separate frontend api to backend internal api #465

Open

jameshartig requested changes Jan 31, 2025

View reviewed changes

frame.go Outdated Show resolved Hide resolved

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from e963b69 to 0ecbf44 Compare February 3, 2025 09:20

OleksiienkoMykyta requested a review from jameshartig February 3, 2025 09:47

jameshartig requested changes Feb 7, 2025

View reviewed changes

frame.go Outdated Show resolved Hide resolved

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch 2 times, most recently from f47bf6a to 9a00dde Compare February 26, 2025 13:20

OleksiienkoMykyta requested a review from jameshartig February 26, 2025 13:22

OleksiienkoMykyta requested a review from worryg0d February 26, 2025 13:31

jameshartig approved these changes Mar 2, 2025

View reviewed changes

CASSGO-26 consistency serial was added

9029339

The user should be able to set consistency to SERIAL or LOCAL_SERIAL for Paxos reads, but the previous implementation doesn't support such a feature. patch by Mykyta Oleksiienko; reviewed by João Reis and James Harting for CASSGO-26

OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch from 9a00dde to 9029339 Compare March 3, 2025 07:38

joao-r-reis merged commit 0aa180b into apache:trunk Mar 18, 2025
16 checks passed

CASSGO-26 consistency serial was added #1837

CASSGO-26 consistency serial was added #1837

Uh oh!

Conversation

OleksiienkoMykyta commented Oct 28, 2024

Uh oh!

joao-r-reis left a comment

Choose a reason for hiding this comment

Uh oh!

OleksiienkoMykyta commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joao-r-reis commented Nov 8, 2024

Uh oh!

joao-r-reis commented Nov 8, 2024

Uh oh!

joao-r-reis Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

OleksiienkoMykyta Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

joao-r-reis commented Nov 12, 2024

Uh oh!

OleksiienkoMykyta commented Nov 12, 2024

Uh oh!

ribaraka Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ribaraka Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

ribaraka Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

OleksiienkoMykyta Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

jameshartig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jameshartig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jameshartig commented Mar 2, 2025

Uh oh!

OleksiienkoMykyta commented Mar 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

OleksiienkoMykyta commented Nov 8, 2024 •

edited

Loading

ribaraka Nov 13, 2024 •

edited

Loading